Skip to content

Conversation

@whoami730
Copy link
Contributor

Fixes #41045.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

"reportOptionalOperand": "warning",
"reportMissingImports": "warning",
"reportMissingModuleSource": "warning",
"strictParameterNoneValue": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many methods in cachefunc.pyi and otherwise as well use f=None and similar definitions.

@github-actions
Copy link

github-actions bot commented Oct 27, 2025

Documentation preview for this PR (built with commit f9f140f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mantepse
Copy link
Contributor

I don't quite understand: are the changes below misc part of this PR?

return rich_to_bool(op, 0)
return rich_to_bool(op, -1)

def isdisjoint(self, other):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard in Sage is to add underscores between separate words in method names.

Suggested change
def isdisjoint(self, other):
def is_disjoint(self, other):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest not to do this in this very special case, because Set is supposed to be a "better" set (it is not, in my opinion, but it tries), and it is set.isdisjoint rather than set.is_disjoint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I have to take this back, the situation in sage is already chaotic :-(

Set (and FrozenBitset) indeed implements issubset etc., but there are some classes (MutablePoset, ManifoldSubset, RealSet, I couldn't find any others) that implement is_subset. In #41113, another class is going to implement is_subset.

It is not clear to me what's best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose fundamentally we should make a choice and then rename/deprecate. For the time being, implementing both could be alright (it's just an extra line isdisjoint = is_disjoint in the class).

@whoami730 whoami730 requested review from mantepse and yyyyx4 November 8, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.isdisjoint() method is missing in Set

3 participants